KTOR-9203 Add non-blocking release function for multi-part content#5372
KTOR-9203 Add non-blocking release function for multi-part content#5372
Conversation
📝 WalkthroughWalkthroughPartData gained a suspendable release callback ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
b59554c to
e0bb735
Compare
e0bb735 to
aa6ab25
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
ktor-http/common/src/io/ktor/http/content/Multipart.kt (1)
17-26: Update KDoc to documentreleaseinstead of deprecateddispose.The KDoc at line 18 mentions
@property disposebutdisposeis now deprecated. Update the documentation to reflect the newreleaseproperty.Proposed KDoc update
/** * Represents a multipart/form-data entry. Could be a [FormItem] or [FileItem]. * * [Report a problem](https://ktor.io/feedback/?fqname=io.ktor.http.content.PartData) * - * `@property` dispose to be invoked when this part is no longer needed + * `@property` release suspend function to be invoked when this part is no longer needed * `@property` headers of this part, could be inaccurate on some engines */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-http/common/src/io/ktor/http/content/Multipart.kt` around lines 17 - 26, Update the KDoc for the sealed class PartData to stop documenting the deprecated dispose property and instead document the new suspend release property: replace the `@property` dispose line with an `@property` release entry that describes its purpose (a suspend function to be invoked when the part is no longer needed) and note that dispose is deprecated and release should be used; reference the PartData class and its release: suspend () -> Unit signature so readers can find the correct member.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ktor-http/api/ktor-http.api`:
- Around line 1517-1547: The public constructors for
io.ktor.http.content.PartData and nested classes (PartData$BinaryChannelItem,
PartData$BinaryItem, PartData$FileItem, PartData$FormItem) were changed to add a
Function1 `release` parameter which breaks binary compatibility; restore
compatibility by adding overload constructors that match the old signatures and
delegate to the new constructors (e.g., provide constructors without the
`release` Function1 that supply a default no-op release), or if the change is
intentional make it explicit in the API by marking it as a breaking change and
add clear migration notes in the public docs and changelog referencing the
modified symbols PartData.<init>, PartData$BinaryChannelItem.<init>,
PartData$BinaryItem.<init>, PartData$FileItem.<init>, and
PartData$FormItem.<init>.
In `@ktor-http/ktor-http-cio/common/src/io/ktor/http/cio/CIOMultipartDataBase.kt`:
- Line 35: The non-blocking cleanup is not actually wired up: update eventToData
to call event.releaseSuspend() instead of the blocking event.release(), and in
partToData when creating FormItem and FileItem supply the suspend release
callback (release = { part.releaseSuspend() }) instead of using the old
disposable lambda (dispose); this ensures previousPart?.release() will invoke
the suspend release provided by those items and removes remaining blocking calls
in eventToData and partToData.
In `@ktor-http/ktor-http-cio/common/src/io/ktor/http/cio/Multipart.kt`:
- Around line 88-91: In releaseSuspend ensure the body channel is always
discarded even if headers.await() throws: wrap the headers.await().release()
call in a try block and call body.discard() in a finally block so body.discard()
runs regardless of exceptions; update the releaseSuspend implementation (the
suspend method named releaseSuspend) to use try/finally around headers.await()
and headers.await().release() while still calling body.discard() in the finally.
---
Nitpick comments:
In `@ktor-http/common/src/io/ktor/http/content/Multipart.kt`:
- Around line 17-26: Update the KDoc for the sealed class PartData to stop
documenting the deprecated dispose property and instead document the new suspend
release property: replace the `@property` dispose line with an `@property` release
entry that describes its purpose (a suspend function to be invoked when the part
is no longer needed) and note that dispose is deprecated and release should be
used; reference the PartData class and its release: suspend () -> Unit signature
so readers can find the correct member.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b92734a-1156-4f5b-b41e-dbb61988a7d6
📒 Files selected for processing (12)
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/MultiPartFormDataTest.ktktor-http/api/ktor-http.apiktor-http/api/ktor-http.klib.apiktor-http/common/src/io/ktor/http/content/Multipart.ktktor-http/ktor-http-cio/api/ktor-http-cio.apiktor-http/ktor-http-cio/api/ktor-http-cio.klib.apiktor-http/ktor-http-cio/common/src/io/ktor/http/cio/CIOMultipartDataBase.ktktor-http/ktor-http-cio/common/src/io/ktor/http/cio/Multipart.ktktor-server/ktor-server-core/common/src/io/ktor/server/engine/DefaultTransform.ktktor-server/ktor-server-plugins/ktor-server-content-negotiation/jvm/test/ContentNegotiationJvmTest.ktktor-server/ktor-server-tests/common/test/io/ktor/tests/server/http/TestEngineMultipartTest.ktktor-server/ktor-server-tests/jvm/test/io/ktor/server/http/MultipartServerTest.kt
1bde154 to
918833b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ktor-http/common/src/io/ktor/http/content/Multipart.kt (1)
23-28: ⚡ Quick winDocument the new
releasecallback onPartData.
disposeis now deprecated, but the class KDoc still only describes the old cleanup path. Please update the public docs so callers can discover thatreleaseis the preferred lifecycle hook.Suggested doc update
- * `@property` dispose to be invoked when this part is no longer needed + * `@property` dispose legacy blocking cleanup callback + * `@property` release suspendable cleanup callback used by the new API🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-http/common/src/io/ktor/http/content/Multipart.kt` around lines 23 - 28, Update the KDoc for the sealed class PartData to document the new suspend release callback as the preferred cleanup lifecycle hook and mark dispose as deprecated; specifically, revise the class-level and constructor parameter docs to explain that release is the canonical asynchronous cleanup method to be invoked by consumers (and may be called automatically by the framework), that dispose remains for binary compatibility but is deprecated, and include usage guidance and any concurrency/exception semantics for release; reference the PartData class and its constructor parameters release and dispose when updating the documentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ktor-http/common/src/io/ktor/http/content/Multipart.kt`:
- Around line 23-28: Update the KDoc for the sealed class PartData to document
the new suspend release callback as the preferred cleanup lifecycle hook and
mark dispose as deprecated; specifically, revise the class-level and constructor
parameter docs to explain that release is the canonical asynchronous cleanup
method to be invoked by consumers (and may be called automatically by the
framework), that dispose remains for binary compatibility but is deprecated, and
include usage guidance and any concurrency/exception semantics for release;
reference the PartData class and its constructor parameters release and dispose
when updating the documentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 15ff66e4-7dc0-486c-940a-6872e11d8ca9
📒 Files selected for processing (12)
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/MultiPartFormDataTest.ktktor-http/api/ktor-http.apiktor-http/api/ktor-http.klib.apiktor-http/common/src/io/ktor/http/content/Multipart.ktktor-http/ktor-http-cio/api/ktor-http-cio.apiktor-http/ktor-http-cio/api/ktor-http-cio.klib.apiktor-http/ktor-http-cio/common/src/io/ktor/http/cio/CIOMultipartDataBase.ktktor-http/ktor-http-cio/common/src/io/ktor/http/cio/Multipart.ktktor-server/ktor-server-core/common/src/io/ktor/server/engine/DefaultTransform.ktktor-server/ktor-server-plugins/ktor-server-content-negotiation/jvm/test/ContentNegotiationJvmTest.ktktor-server/ktor-server-tests/common/test/io/ktor/tests/server/http/TestEngineMultipartTest.ktktor-server/ktor-server-tests/jvm/test/io/ktor/server/http/MultipartServerTest.kt
✅ Files skipped from review due to trivial changes (1)
- ktor-server/ktor-server-tests/jvm/test/io/ktor/server/http/MultipartServerTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- ktor-server/ktor-server-plugins/ktor-server-content-negotiation/jvm/test/ContentNegotiationJvmTest.kt
- ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/MultiPartFormDataTest.kt
Subsystem
Server, Core
Motivation
KTOR-9203 CIOMultipartDataBase: Call thread is blocked when releasing file parts since 3.2.0
Solution
Added function for releasing multi-part contents with suspend instead of blocking.